added rate limiting #221
added rate limiting #221
Conversation
|
This is merging into a branch (dashboard updates) right now since it was easier for my environment to work on this way. Once that other branch gets merged in, this could get merged into master. |
| async def check_rate_limit(user_id): | ||
| '''Reusable rate limiter with service-specific settings''' | ||
| # TODO fetch from pmss/define appropriate window | ||
| max_requests = 2 |
There was a problem hiding this comment.
Something more descriptive.
max_user_requests_per_window
|
|
||
|
|
||
| def create_rate_limiter(service_name): | ||
| '''Factory function for rate limiters with closure over service name''' |
There was a problem hiding this comment.
That repeats the function name. I would describe what the rate limiter does (e.g. max # of requests per window). Basically, autogen docs should tell me what this does.
| now = time.time() | ||
|
|
||
| # Initialize user/service tracking | ||
| key = f'rate_limit:{service_name}:{user_id}' |
| RATE_LIMITERS[key] = collections.deque() | ||
|
|
||
| # Expire old requests | ||
| timestamps = RATE_LIMITERS[key] |
There was a problem hiding this comment.
request_timestamps, maybe? or prior_requests_timestamps?
|
|
||
|
|
||
| def rate_limited(service_name): | ||
| '''Decorator for async functions needing rate limiting''' |
There was a problem hiding this comment.
This should clearly document what kinds of functions this can wrap, and the protocol. Things like this:
if 'runtime' not in kwargs:
raise TypeError(f'`{func.__name__}` requires `runtime` keyword argument for checking rate limits.')
Should be in the docstring. Basically, I want to know how I use this.
|
|
||
| runtime = kwargs['runtime'] | ||
|
|
||
| check_limit = create_rate_limiter(service_name) |
There was a problem hiding this comment.
check_rate_limit would be slightly nicer.
| user = await learning_observer.auth.get_active_user(request) | ||
| user_id = user[learning_observer.constants.USER_ID] | ||
| if not await check_limit(user_id): | ||
| raise PermissionError(f'Rate limit exceeded for {service_name} service') |
There was a problem hiding this comment.
I'm actually a bit confused when I'd want this behavior. It'd be helpful to know when this is used.
Seems like what I want:
- Queue up requests
- If they go obsolete (e.g. user navigates away), drop them
- If they don't, let the user know we're throttling and throttle them
Simply failing seems like it might be annoying to the user.
|
I did a review. The code is fine on a code level, but I'm concerned on an algorithm level. I think what we want is:
|
2 participants
Added a generic rate limiting decorator.
TODO